Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Add IDV attempt events #385

Merged
merged 12 commits into from
Sep 5, 2024
Merged

feat: Add IDV attempt events #385

merged 12 commits into from
Sep 5, 2024

Conversation

ilee2u
Copy link
Contributor

@ilee2u ilee2u commented Aug 22, 2024

This PR is a draft for now while my team looks over this.

Description: Adding events for status changes to the new VerificationAttempt model planned for edx-platform.

ISSUE: openedx/platform-roadmap#367

Dependencies: None.

Merge deadline: None.

Keeping the rest of this template as is for now.

Reviewers:

  • tag reviewer
  • tag reviewer

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed

Post merge:

  • Create a tag
  • Check new version is pushed to PyPI after tag-triggered build is
    finished.
  • Delete working branch (if not needed anymore)

Author concerns: List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.

@ilee2u ilee2u marked this pull request as ready for review August 26, 2024 20:15
@ilee2u ilee2u requested a review from a team as a code owner August 26, 2024 20:15
@ilee2u
Copy link
Contributor Author

ilee2u commented Aug 26, 2024

Note for open source reviewers: These events were proposed and approved as part of the Open edX proposal associated with the platform-roadmap ticket in the pull request description.

MichaelRoytman
MichaelRoytman suggested changes Aug 26, 2024
View reviewed changes
docs/decisions/0016-idv-attempt-events.rst Outdated Show resolved Hide resolved
docs/decisions/0016-idv-attempt-events.rst Outdated Show resolved Hide resolved
openedx_events/learning/data.py Outdated Show resolved Hide resolved
openedx_events/learning/signals.py Outdated Show resolved Hide resolved
openedx_events/learning/data.py Outdated Show resolved Hide resolved
openedx_events/learning/data.py Outdated Show resolved Hide resolved
docs/decisions/0016-idv-attempt-events.rst Outdated Show resolved Hide resolved
docs/decisions/0016-idv-attempt-events.rst Outdated Show resolved Hide resolved
openedx_events/learning/data.py Outdated Show resolved Hide resolved
@ilee2u ilee2u force-pushed the ilee2u/idv-attempt-event branch from 7044595 to c1984ea Compare August 28, 2024 18:27
MichaelRoytman
MichaelRoytman approved these changes Aug 28, 2024
View reviewed changes
Copy link

@MichaelRoytman MichaelRoytman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I left a few comments I'd like addressed, but I'll approve the pull request. We shouldn't merge this until we get feedback from the owners, though.

openedx_events/learning/data.py Outdated Show resolved Hide resolved
openedx_events/learning/data.py Outdated Show resolved Hide resolved
@mariajgrimaldi
Copy link
Member

Thank you, folks, for this contribution. I look forward to seeing it implemented!

mariajgrimaldi
mariajgrimaldi approved these changes Aug 30, 2024
View reviewed changes
Copy link
Member

@mariajgrimaldi mariajgrimaldi left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a minor suggestion. Also, I think one comment from Michael might need your attention. Thank you both for providing the team with such detailed context in the proposal. I'll leave my approval since this implementation aligns with what you proposed and the implementation on the edx-platform openedx/edx-platform#35311.

I'll merge this once all comments are addressed. Thanks!

openedx_events/learning/data.py Outdated Show resolved Hide resolved
openedx_events/learning/signals.py Outdated Show resolved Hide resolved
openedx_events/learning/signals.py Outdated Show resolved Hide resolved
openedx_events/learning/signals.py Outdated Show resolved Hide resolved
openedx_events/learning/signals.py Outdated Show resolved Hide resolved
mariajgrimaldi
mariajgrimaldi approved these changes Sep 4, 2024
View reviewed changes
Copy link
Member

@mariajgrimaldi mariajgrimaldi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ilee2u, thank you. Sorry to bother you but can you please add an entry to the changelog file? Thanks!

@ilee2u
Copy link
Contributor Author

ilee2u commented Sep 5, 2024

@ilee2u, thank you. Sorry to bother you but can you please add an entry to the changelog file? Thanks!

@mariajgrimaldi Just added this!

@mariajgrimaldi mariajgrimaldi merged commit f790bd4 into openedx:main Sep 5, 2024
11 checks passed
@ilee2u ilee2u deleted the ilee2u/idv-attempt-event branch September 5, 2024 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mariajgrimaldi mariajgrimaldi mariajgrimaldi approved these changes

@MichaelRoytman MichaelRoytman MichaelRoytman approved these changes

@felipemontoya felipemontoya Awaiting requested review from felipemontoya

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ilee2u @mariajgrimaldi @MichaelRoytman